Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove auto self-contained build for .NET 8 beanstalk deployments #847

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

philasmar
Copy link
Contributor

Issue #, if available:
DOTNET-7212

Description of changes:
In order to support deploying .NET8 apps on Beanstalk prior to Beanstalk adding .NET8 support, we had to force .NET8 apps to be self-contained in order to allow for Beanstalk support. Beanstalk has since added .NET8 support, so this PR removes this restriction to allow .NET8 apps to be deployed to Beanstalk not as self-contained apps.

This PR also improves the sorting of Beanstalk platforms to show the latest .NET versions first.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@philasmar philasmar requested review from ashovlin, normj and 96malhar July 19, 2024 19:43
@philasmar philasmar force-pushed the asmarp/add-net8-beanstalk branch from 24c3d67 to 8ba5823 Compare July 19, 2024 19:53
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 6 lines in your changes missing coverage. Please review.

Project coverage is 62.39%. Comparing base (db9ec45) to head (746a0e7).
Report is 1 commits behind head on dev.

Files Patch % Lines
...WS.Deploy.Orchestration/DeploymentBundleHandler.cs 86.11% 1 Missing and 4 partials ⚠️
...WS.Deploy.Orchestration/Data/AWSResourceQueryer.cs 95.83% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #847      +/-   ##
==========================================
+ Coverage   61.93%   62.39%   +0.45%     
==========================================
  Files         279      279              
  Lines       10850    10900      +50     
  Branches     1496     1513      +17     
==========================================
+ Hits         6720     6801      +81     
+ Misses       3598     3563      -35     
- Partials      532      536       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@philasmar
Copy link
Contributor Author

Add a platform version check for the version that added .NET8. If a user is redeploying we shouldn't break them.

@96malhar 96malhar self-requested a review July 24, 2024 20:22
@philasmar philasmar force-pushed the asmarp/add-net8-beanstalk branch 4 times, most recently from 8d2d690 to ca7d45d Compare July 26, 2024 15:47
@philasmar philasmar force-pushed the asmarp/add-net8-beanstalk branch from ca7d45d to 1ccd396 Compare July 29, 2024 14:47
Comment on lines 141 to 145
if (beanstalkPlatformSettingValueSplit?.Length != 3)
throw new InvalidElasticBeanstalkPlatformException(DeployToolErrorCode.InvalidElasticBeanstalkPlatform, $"The selected Elastic Beanstalk platform version '{beanstalkPlatformSettingValue}' is invalid.");
var beanstalkPlatformName = beanstalkPlatformSettingValueSplit[1];
if (!Version.TryParse(beanstalkPlatformSettingValueSplit[2], out var beanstalkPlatformVersion))
throw new InvalidElasticBeanstalkPlatformException(DeployToolErrorCode.InvalidElasticBeanstalkPlatform, $"The selected Elastic Beanstalk platform version '{beanstalkPlatformSettingValue}' is invalid.");
Copy link
Member

@ashovlin ashovlin Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this format part of Elastic Beanstalk's "contract"? I would hate to penalize users by throwing an exception if EB ever changed the convention of these strings.

I'm wondering if it'd be safer to just proceed if we fail to parse, and possibly let the deployment fail, since users could then set Self Contained Build themselves to proceed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@96malhar 96malhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved assuming you address Alex's feedback

@philasmar philasmar requested a review from ashovlin August 2, 2024 19:26
@philasmar philasmar merged commit a9ffccb into dev Aug 5, 2024
11 checks passed
@philasmar philasmar deleted the asmarp/add-net8-beanstalk branch August 5, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants